Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Core Systems Refactor #377

Merged
merged 83 commits into from
Feb 20, 2024
Merged

Core Systems Refactor #377

merged 83 commits into from
Feb 20, 2024

Conversation

Tides
Copy link
Member

@Tides Tides commented Jun 18, 2023

One of the current changes is how we handled injecting/registering services from clients. I also want to discuss more about how we handle/register events and try to move away from scanning method names for events.

Draft for now as we work this out.

As this refactor evolved, so did the systems relying on it. I did my best to keep the changes within the three big systems (PluginManager, CommandHandler and EventDispatcher).

And since I'm approaching the end of this refactor, I'd like to add the most major changes to the various systems (Events and Commands).

PluginManager

The entire class and a majority of the things that relied on it have changed.
Here's moreso a list of things that changed.

  • A service collection was created for plugins
  • Each plugin container gets a scope of that service collection and is disposed when the plugin is disposed.
  • Plugins have two new methods "ConfigureServices" and "ConfigureRegistry"
  • All reflection methods were removed from PluginBase
  • PluginDependency was removed in favor of the shared ServiceCollection
  • PluginInfo is now scanned and populated from a plugin.json embedded resource. Sample Plugin

When plugins are "handled" (e.x loaded and reloaded), two methods will be called. "ConfigureServices" and "ConfigureRegistry".

ConfigureServices

Is, as the name explains, configuring services. These services will be added to a shared service collection and will be turned into a scope service provider for every plugin when the server is fully loaded.

ConfigureRegistry

Isn't as self-explanatory, at least to those unfamiliar with any type of Minecraft modding. This method is used for registering commands, events, and eventually items, blocks, entities, biomes, dimensions, and many more. And it gets called after ConfigureRegistry.

ObjectMethodExecutor

This Post explains best why I decided to bring this in. But both EventDispatcher and CommandHandler use ObjectMethodExecutor.

The Command Framework

The way commands are stored and executed has changed, while the system itself is more or less the same (finding and registering). Any problems we had before with commands, at least with my current testing, are not present anymore. We now support registering a "minimal API" way of registering commands as well.

registry.MapCommand("test", 
    [CommandInfo("test command")]
    async (CommandContext ctx, int number, int otherNumber) =>
    {
        await ctx.Player.SendMessageAsync("Test #{number} and #{otherNumber}. This command was executed from the MinimalAPI.");
    });

Attributes and parameters are found and are read accordingly by the framework. You're not able to register command groups yet through this API, but I do plan on adding it in the future.

The way to register command modules has also changed a bit. You no longer need to provide a CommandContext parameter in your command methods as it is included as a property in the new CommandModuleBase abstract class, which all your commands should inherit from. Dependency injection works normally, either through the inject attribute or through constructors and eventually parameter injection. These classes have a scoped lifetime.

public class MyCommands : CommandModuleBase
{
    [Inject]
    public ILogger<MyCommands> Logger { get; set; }

    [Command("mycommand")]
    [CommandInfo("woop dee doo this command is from a plugin")]
    public async Task MyCommandAsync()
    {
        Logger.LogInformation("Testing Services as injected dependency");
        await this.Player.SendMessageAsync("Hello from plugin command!");
    }
}

Command Handler Internals a few things changed with this but not a lot.

The Event System

The way events are stored and executed has changed, but not by a lot. Instead of scanning method names to try and match them to current events and then registering them through the traditional event subscription model, we are now checking the first parameter of the event and then adding that method to a list of executors stored in a dictionary stored by event names if it is a valid event type. The executors are then executed in ASC order of priority.

And as registering commands has a similar "minimal API" approach, so do events.

registry.MapEvent((IncomingChatMessageEventArgs chat) =>
{
    this.Logger.LogDebug("Got a chat message! From MinimalAPI event.");
});

The way to register event handlers is simple. Inherit from the MinecraftEventHandler abstract class, and the PluginManager will scan and store these for you. Dependency injection works normally, either through the inject attribute or through constructors and eventually parameter injection. These classes have a scoped lifetime as well.

public class MyEventHandler : MinecraftEventHandler
{
    [Inject]
    public ILogger<MyEventHandler> Logger { get; set; }

    [EventPriority(Priority = Priority.Critical)]
    public async ValueTask ChatEvent(IncomingChatMessageEventArgs args)
    {
        Logger.LogInformation("Wow a message through the event handler class!");
        await args.Player.SendMessageAsync("I got your chat message through the event handler class!");
    }
}

Here's the internals

Final stuff thingys

hopefully this will set the path for future implementations and ease up a lot of pain introduced by the old implementations.

@Tides Tides marked this pull request as draft June 18, 2023 04:47
@github-actions github-actions bot added api Relates to Obsidian.API commands Relates to Obsidian.Commands plugins Relates to plugins labels Jun 18, 2023
@Seb-stian
Copy link
Member

When it comes to registering event handlers, we must have the following in mind:

  • IDE support
    • discoverability via code completion
    • checking for correct signature
  • support asynchronous code (handlers should return ValueTask or ValueTask<>)
  • forward-compatibility - it should be possible to add events while keeping old plugins (source and binary) compatible
  • enable cancellation of default behaviour

There was also a discussion about whether and how should order of handling events be determined.

@Tides Tides mentioned this pull request Nov 18, 2023
@github-actions github-actions bot added source-generation Relates to Obsidian.SourceGenerators tests Relates to Obsidian.Tests labels Dec 11, 2023
Craftplacer
Craftplacer previously approved these changes Dec 11, 2023
Obsidian/Plugins/PluginManager.cs Outdated Show resolved Hide resolved
@Tides Tides marked this pull request as ready for review January 25, 2024 18:05
Copy link
Member

@Naamloos Naamloos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All in all, seems clean. Check my feedback, notices some minor oversights.

Obsidian.API/Commands/CommandModuleBase.cs Show resolved Hide resolved
Obsidian.API/Utilities/CommandHelpers.cs Show resolved Hide resolved
Obsidian.API/_Enums/Priority.cs Show resolved Hide resolved
Obsidian/Client.cs Outdated Show resolved Hide resolved
Obsidian/Client.cs Show resolved Hide resolved
Obsidian/Registries/CommandsRegistry.cs Show resolved Hide resolved
Obsidian/Services/EventDispatcher.cs Outdated Show resolved Hide resolved
Naamloos
Naamloos previously approved these changes Jan 27, 2024
@Tides Tides changed the title Plugin manager refactor Core Systems Refactor Feb 10, 2024
@Naamloos
Copy link
Member

lil mergie conflictie

@Tides
Copy link
Member Author

Tides commented Feb 16, 2024

lil mergie conflictie

I'll fix em rq

@Naamloos Naamloos merged commit 7925111 into master Feb 20, 2024
2 checks passed
@Tides Tides deleted the plugin-manager-cleanup branch December 7, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
api Relates to Obsidian.API commands Relates to Obsidian.Commands plugins Relates to plugins source-generation Relates to Obsidian.SourceGenerators tests Relates to Obsidian.Tests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants